Skip to content

fix: allow text after any tool call #6013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
daniel-lxs wants to merge 3 commits into from
Closed

fix: allow text after any tool call #6013

daniel-lxs wants to merge 3 commits into from

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Jul 21, 2025 •
edited by roomote bot
Loading

Summary

This PR fixes issue #5847 by allowing text to be rendered after ANY tool use.

Changes

src/core/assistant-message/presentAssistantMessage.ts

  • Line 85: Removed cline.didAlreadyUseTool check from text block processing – now text can appear after any tool use.

src/core/task/Task.ts

  • No changes needed for text rendering after tools.

Behavior Changes

Before:

  • ❌ No text could appear after any tool use
  • ❌ All tools limited to one use per message
  • ❌ "Response interrupted by a tool use result" message appeared for all tools

After:

  • ✅ Text can appear after ANY tool use
  • ✅ All tools remain limited to one per message (including update_todo_list)
  • ✅ "Response interrupted by a tool use result" message only appears when multiple tools are attempted

Screenshot:

Screenshot_20250721_082501 Screenshot_20250721_082440

Related Issues

Fixes #5847

@daniel-lxs
fix: allow text content after update_todo_list tool (#5847)
4f55493 
The update_todo_list tool is a special case that doesn't produce actionable output or require user interaction beyond confirmation. Unlike other tools that perform file operations or system commands, update_todo_list simply updates the UI's todo list display.

This change exempts update_todo_list from setting the didAlreadyUseTool flag, which allows subsequent text content to be rendered. This enables the assistant to provide context or explanations after updating the todo list, improving the user experience.

The one-tool-per-message restriction remains in effect for all other tools to prevent multiple potentially conflicting operations in a single message.
@daniel-lxs daniel-lxs requested review from cte, jr and mrubens as code owners July 21, 2025 13:36
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Jul 21, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Jul 21, 2025
@delve-auditor
Copy link

delve-auditor bot commented Jul 21, 2025 •
edited
Loading

✅ No security or compliance issues detected. Reviewed everything up to d2c1a26.

Security Overview
  • 🔎 Scanned files: 2 changed file(s)
Detected Code Changes
Change Type Relevant files
Enhancement ► presentAssistantMessage.ts
    Modified tool blocking logic for update_todo_list
    Allow multiple update_todo_list calls
► Task.ts
    Updated stream interruption logic for update_todo_list
    Allow text after tool calls

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

mrubens
mrubens approved these changes Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok, but also curious why we need this for other tools. Can we wait until the second tool use to error out?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 21, 2025
@daniel-lxs
Copy link
Member Author

daniel-lxs commented Jul 21, 2025

@mrubens Your comment gives me an idea

@daniel-lxs
fix: allow text after update_todo_list and support multiple calls
d2c1a26 
- Modified presentAssistantMessage.ts to skip tool blocking for update_todo_list
- Prevented setting didAlreadyUseTool flag when update_todo_list is used
- Updated Task.ts to check for update_todo_list before interrupting stream
- Allows update_todo_list to be called multiple times in a single message
- Text can now appear after update_todo_list tool calls

Fixes #5847
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 21, 2025
@daniel-lxs daniel-lxs changed the title fix: allow text content after update_todo_list tool (#5847) fix: allow text after update_todo_list and support multiple calls Jul 21, 2025
@daniel-lxs daniel-lxs changed the title fix: allow text after update_todo_list and support multiple calls fix: allow text after any tool call Jul 21, 2025
mrubens
mrubens reviewed Jul 22, 2025
View reviewed changes
Copy link
Collaborator

@mrubens mrubens left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reflecting on it, I feel a little bit nervous about the move to multiple tool use and any issues that I might not be thinking of. What would you think of starting with a PR to just allow text after a tool use? That seems safer to me.

@daniel-lxs
Copy link
Member Author

daniel-lxs commented Jul 22, 2025

@mrubens
The only tool that can be called with other tools is update_todo_list which technically doesn't have any external influence, but yeah we can start with text only.

@daniel-lxs
Copy link
Member Author

daniel-lxs commented Jul 22, 2025

@roomote-agent Please update this PR to remove the special condition for the update_todo_list to be called alongside other tools, leave everything else untouched and don't create any new tests.

You may update the PR description after you are done, do not create a new PR.

@roomote
Copy link
Contributor

roomote bot commented Jul 22, 2025

I understand. I will update this PR to remove the special condition for update_todo_list to be called alongside other tools, keeping only the change that allows text after any tool use. Starting work on this now.

@roomote
fix: remove special condition for update_todo_list tool
3bce917 
- Remove exception that allowed update_todo_list to be called alongside other tools
- Now all tools are limited to one per message without exceptions
- Keeps the change that allows text after any tool use
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 22, 2025
@roomote
Copy link
Contributor

roomote bot commented Jul 22, 2025

I've successfully updated this PR as requested. Here's what I changed:

Changes Made:

  1. Removed special condition for in :

    • Line 238: Removed the exception that allowed to bypass the tool limit
    • Lines 260-263: Removed the condition that prevented setting for
    • Lines 557-561: Removed the special handling in the streaming completion logic

  2. Removed special condition for in :

    • Lines 1430-1440: Removed the check that allowed to continue when other tools would be interrupted

Result:

The PR description has been updated to reflect these changes. CI checks are currently running.

mrubens
mrubens reviewed Jul 23, 2025
View reviewed changes
src/core/assistant-message/presentAssistantMessage.ts
Comment on lines +554 to +559
if (
!block.partial ||
cline.didRejectTool ||
(cline.didAlreadyUseTool && block.type !== "tool_use") ||
(cline.didAlreadyUseTool && block.type === "tool_use")
) {
Copy link
Collaborator

@mrubens mrubens Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any different than it was before?

Copy link
Member Author

@daniel-lxs daniel-lxs Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows the system to continue processing text blocks even after a tool has been used, while still preventing multiple tool executions in the same message.

Copy link
Collaborator

@mrubens mrubens Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe I'm not seeing it but the logic looks identical

@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 30, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Draft / In Progress] in Roo Code Roadmap Jul 30, 2025
@daniel-lxs daniel-lxs closed this Aug 4, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 4, 2025
@github-project-automation github-project-automation bot moved this from PR [Draft / In Progress] to Done in Roo Code Roadmap Aug 4, 2025
@daniel-lxs daniel-lxs mentioned this pull request Aug 28, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrubens mrubens mrubens approved these changes

@cte cte Awaiting requested review from cte cte is a code owner

@jr jr Awaiting requested review from jr jr is a code owner

Assignees

No one assigned

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Draft / In Progress size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Roo Code Roadmap
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

todo_list tool (or the update that added it) breaks workflows involving user interaction

5 participants

@daniel-lxs @mrubens @hannesrudolph @roomote